Skip to content

VAPI-3354 feat(dtmf): expose duration/interToneGap and fix null safety on RTCDTMFSender#7

Merged
smoghe-bw merged 7 commits into
mainfrom
feat/dtmf-duration-and-null-safety
Jun 24, 2026
Merged

VAPI-3354 feat(dtmf): expose duration/interToneGap and fix null safety on RTCDTMFSender#7
smoghe-bw merged 7 commits into
mainfrom
feat/dtmf-duration-and-null-safety

Conversation

@smoghe-bw

Copy link
Copy Markdown
Contributor

Summary

  • Expose duration and interToneGap on sendDtmf — forwarded directly to RTCDTMFSender.insertDTMF(). The README previously said these could not be controlled, which was wrong for the native browser API.
  • Fix null safety: transceiver.sender.dtmf is RTCDTMFSender | null per spec; the previous ! non-null assertion would throw in environments where DTMF is unsupported for a given track. Now guarded before storing.
  • Add 5 unit tests covering: broadcast (no streamId), targeted (with streamId), parameterised (duration + interToneGap), no-senders noop, and unknown-streamId noop.

Context

Companion to the fix/dtmf-rfc4733-forwarding change in pv-gateway, which now correctly forwards RFC 4733 telephone-event RTP packets. The SDK already used the browser's native RTCDTMFSender (which generates these packets) rather than the Web Audio API approach in dtmfSender.ts; this PR makes the API complete and safe.

Test plan

  • npx jest src/v1/bandwidthRtc.test.ts — all 10 tests pass
  • Manual: call sendDtmf("1", undefined, 200, 80) in a live session and confirm the gateway forwards it as a telephone-event RTP packet with the correct duration

🤖 Generated with Claude Code

…MFSender

- Add `duration` and `interToneGap` params to `sendDtmf` on both the
  public wrapper and v1 implementation, forwarded directly to
  `RTCDTMFSender.insertDTMF()`. README previously stated these could
  not be controlled; that was incorrect for the native browser API.
- Guard `transceiver.sender.dtmf` before storing — the property is
  `RTCDTMFSender | null` per spec and the previous `!` assertion would
  throw in environments where DTMF is unsupported for a track.
- Add five unit tests covering: broadcast, targeted, parameterised,
  no-senders, and unknown-streamId paths.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@smoghe-bw smoghe-bw requested review from a team as code owners June 16, 2026 19:01
@bwappsec

bwappsec commented Jun 16, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@smoghe-bw smoghe-bw changed the title feat(dtmf): expose duration/interToneGap and fix null safety on RTCDTMFSender VAPI-3354 feat(dtmf): expose duration/interToneGap and fix null safety on RTCDTMFSender Jun 16, 2026
Comment thread src/v1/bandwidthRtc.ts Outdated
// this.offerPublishSdp(true);
// connectionState = pc.connectionState;
// }
// TODO: add timeout here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more to this comment to indicate that we need to trigger a ice restart? The commented out offerPublishSdp(true) is an older method to re-publish the existing SDP with restart ice true. I don't want to loose that ref if possible, so uncommenting this block and putting it behind a global retryIceOnFailed with a constant false for now would be good.

@dhelms-bw dhelms-bw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change to not lose the ice renegotiation behavior.

@smoghe-bw smoghe-bw merged commit a750236 into main Jun 24, 2026
5 checks passed
@smoghe-bw smoghe-bw deleted the feat/dtmf-duration-and-null-safety branch June 24, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants